fix: prevent report output symlink races - #798

[SkillSpector Review]

Hi @yashrajp22, thank you for routing every CLI report and baseline write through a single descriptor-relative writer! The no-follow parent walk, the O_EXCL temp file and the dir_fd replace close the leaf-swap, parent-swap and hard-link cases together, and the parametrised race test exercises each of them deterministically.

Value and readiness: The problem is real on main. With main's own code, _write_result and dump_baseline writing through a leaf symlink overwrote an external sentinel, a hard-linked destination was modified in place, and scan --mcp-registry and scan --recursive --format json with a symlinked --output exited 0 and overwrote the sentinel. The most realistic case also reproduces: skillspector baseline . --no-llm inside a checkout that ships .skillspector-baseline.yaml -> ../outside/victim_rc (the default -o path) exited 0 and overwrote victim_rc. I transcribed file_output.py:27-85 and the changed call sites and ran them with main's CLI on macOS. Every one of these cases is now refused or contained, report bytes are unchanged, and the planted-baseline checkout exits 2 with victim_rc intact. All seven write sites are covered and the exit-2 contract on write errors is kept. One blocker remains: on Windows, which the project supports, skillspector baseline stops working entirely, its error points to stdout redirection that baseline does not offer, and both baseline and scan --output fail only after the full scan has run (finding 1). Findings 2-8 are non-blocking; 2-4 (error messages, file mode and newly refused targets) and 5 (tests for the baseline, recursive and registry writers) are the most useful to address here. Once finding 1 is resolved, the PR should be ready for final maintainer review.

Material findings

1. [Blocker] src/skillspector/file_output.py:42: on Windows, baseline becomes unusable and every --output write fails only after the scan, with a hint that baseline cannot follow.
   • Windows CPython has no os.O_NOFOLLOW or os.O_DIRECTORY and an empty os.supports_dir_fd (per the CPython docs; not run on Windows). So _SECURE_OUTPUT_SUPPORTED (file_output.py:27-32) is False, and lines 42-45 raise "Safe file output is unsupported on this platform; use stdout redirection." Every writer reaches it: cli.py:363, 697, 3236, 3264, 3297 and suppression.py:650/656.
   • baseline --output is a plain Path defaulting to .skillspector-baseline.yaml (cli.py:3373-3380), with no stdout mode. graph.invoke (cli.py:3420, LLM analysis on by default) runs before dump_baseline (cli.py:3433), and the error becomes exit 2 at cli.py:3439-3441. scan calls _write_result at cli.py:857, after scan_skill. Nothing checks platform support before the scan; the only pre-scan output check is the samefile guard at cli.py:717-727.
   • Measured with main's CLI in-process, the writers replaced by a transcription of the PR code and the gate forced False: baseline <skill> --no-llm -o b.yaml, baseline with the default path, and scan --no-llm -f json --output r.json each ran the graph once, then exited 2 with that message and wrote nothing. main exits 0 and writes the file in all three cases. With the gate True, the transcription succeeds.
   • Windows is a supported target: pyproject.toml:24 declares "OS Independent", input_handler.py:314-315 routes Windows reads to a reparse-point-safe _open_regular_file_from_windows_handle instead of refusing, the 2.12.0 notes list Windows fixes (#484, #503, #518), and both extensions have win32 branches and pass --output (extensions/skillspector.ts:112, .opencode/tools/skillspector_scan_lib.ts:216). CI is ubuntu-only, so nothing catches this. tests/unit/test_suppression.py:713/736 and about 33 pre-existing --output/-o invocations in test_cli.py would fail on Windows.
   • Consequence: the PR body discloses the fail-closed scan --output behaviour, and stdout is a workable fallback there (an agent can re-run the extension tool without output). baseline has no workaround: a documented command (README.md:239, docs/SUPPRESSION.md:37) disappears on Windows, the message suggests something impossible, the scan and any LLM cost are spent before the failure, and nothing in the user docs says so.
   • Expected fix (minimal, keeps failing closed):
     • Expose a support check from file_output and call it before any analysis when an output file is requested: in scan next to the pre-scan check at cli.py:717-727 (this covers single-skill, --recursive and --mcp-registry) and in baseline before graph.invoke. Exit 2 with a command-specific message.
     • Give baseline a stdout mode (for example -o -), or at least stop suggesting stdout redirection for it.
     • Add baseline and recursive unsupported-platform tests that assert no scan ran and no file was written, and skip or monkeypatch the existing file-output tests on unsupported platforms.
     • Document the limitation in the README --output section and docs/SUPPRESSION.md.
     • Alternatives for the PIC: keep main's plain write with a warning on unsupported platforms (no worse than main), or add a native Windows writer as a follow-up. A Windows writer that checks the parent by handle and then calls os.replace by path is not race-free, because the rename resolves the path again. It would need to hold each ancestor handle open without FILE_SHARE_DELETE, or rename relative to the parent handle (SetFileInformationByHandle with FileRenameInfo).
2. [Non-blocking] src/skillspector/file_output.py:52: an output path through a symlinked directory now fails after the scan with "[Errno 20] Not a directory: ''".
   • Only root-owned aliases directly under / are normalised (file_output.py:46, via input_handler.py:251-262). Every other ancestor is opened with O_DIRECTORY|O_NOFOLLOW at lines 51-54 with no error mapping, whereas input_handler.py:340-344 maps the same errors to messages that carry the full path.
   • Measured (transcription in main's CLI, macOS): scan <skill> --no-llm -f json -o <abs>/plink/r.json and baseline -o <abs>/plink/b.yaml, with plink a user-owned symlink to a directory, both exit 2 with "Error: [Errno 20] Not a directory: 'plink'" and write nothing; main exits 0. A relative plink/r2.json behaves the same. A missing parent already failed on main (exit 2, naming missingdir/r.json); the PR names only missingdir.
   • Scope: only paths that spell out a symlinked component are affected, such as "$PWD/..." or ~/link/... when those cross a symlink. A relative path from a cwd reached through a symlink still works, because os.path.abspath uses the physical cwd. The Pi and OpenCode extensions are not affected. Linux was not run; the same walk on the input side suggests ENOTDIR or ELOOP there.
   • Consequence: refusing symlinked parents is intended (the parent-before-open test expects it), but the error calls a directory "not a directory", names a single component, and the finished report is lost. The PR body's "Explicit regular output paths outside the workspace remain supported" does not hold for these paths.
   • Expected fix: catch OSError around the walk. Map ELOOP/ENOTDIR to a message naming the full output path (for example "Refusing to write output through a symlinked directory: ") and FileNotFoundError to "Output directory does not exist: ". Optionally run a cheap no-follow pre-check before scanning, keeping the write-time walk as the real guard. Document the refusal and correct the PR body.
3. [Non-blocking] src/skillspector/file_output.py:66: reports and baselines are always written 0600, including rewrites of existing 0644 files.
   • The temp file is created with mode 0o600 (lines 63-68) and os.replace installs that inode; nothing restores the mode.
   • Measured (transcription vs main's dump_baseline): under umask 022, a new file is 0644 on main and 0600 with the PR. An existing 0664 file stays 0664 on main (inode kept) and becomes 0600 with the PR; because it is a new inode, owner, group, ACLs and xattrs also reset. Under umask 002 the PR still gives 0600.
   • The README Docker example (README.md:116-121) runs as root, since the Dockerfile has no USER. On rootful Linux Docker the bind-mounted report.json therefore becomes root:root 0600 and the host user cannot read it; on main it is 0644. This is inferred from Docker semantics and was not run; Docker Desktop and rootless setups are unaffected. Git is unaffected (a 0600 file is still stored as 100644), so the shared-baseline impact is limited to other uids reading the same filesystem copy, such as multi-user hosts or mixed-uid CI.
   • Expected fix, either:
     • Create the temp file with 0o666 and let the kernel apply the umask (no os.umask call needed), and call os.fchmod(fd, stat.S_IMODE(existing.st_mode)) when replacing an existing regular file. Update the 0600 assertion at tests/unit/test_cli.py:7109. The O_EXCL and no-follow protections are unchanged.
     • Or keep 0600 as deliberate policy, document it, and add --user "$(id -u):$(id -g)" to the Docker --output example.
4. [Non-blocking] src/skillspector/file_output.py:60: some previously working targets are now refused, and the refusal messages do not name the output path.
   • Lines 60-61 reject any non-regular leaf with "Refusing to overwrite a non-regular output file." (no path, no file type). Lines 62-77 always create a temp file in the parent, so the parent must be writable. macOS has no O_PATH, so line 47 opens every ancestor, the parent included, with O_RDONLY, which needs read permission.
   • Measured (main's real CLI vs main's CLI with a transcribed _write_result, macOS):
     • -o /dev/null: exit 0 on main; exit 2 with the message above on the PR.
     • An existing writable file in a 0555 directory: exit 0 on main; exit 2 with "Permission denied: '.skillspector-output-'" on the PR.
     • A new file under an ancestor at mode 0311: exit 0 on main; exit 2 with "Permission denied: 'outer'" on the PR. This case is macOS/BSD only; Linux uses O_PATH, which needs search permission only.
     • -o /dev/stdout is negligible: main already rejects it with exit 2 when stdout is a file or pipe, and accepts it only on a tty.
   • Consequence: -o /dev/null as an exit-code-only idiom, pre-provisioned output files in read-only directories and restrictive ancestors on macOS now fail, with messages naming a temp file or a single path component. No in-repo caller uses these targets, and the PR body mentions the special-file refusal.
   • Expected fix: include the requested path and file type in the non-regular message, and re-raise write OSErrors with the user's output path. Decide whether to allow character devices (open the existing leaf with O_WRONLY|O_NOFOLLOW when it is S_ISCHR) or document the refusal. Document that the parent must be writable. On macOS, os.O_SEARCH may remove the read requirement on ancestors (it exists on this Python 3.13; not tested as a dirfd here).
5. [Non-blocking] tests/unit/test_cli.py:7138: no regression test covers the baseline, --recursive or --mcp-registry writers.

   • The tests at test_cli.py:7067 and 7115 call write_text_no_follow directly, and both would catch a naive Path.write_text helper. The test at 7138 drives single-skill scan on the unsupported branch, which does protect cli.py:363. Nothing exercises cli.py:697, 3236, 3264, 3297 or suppression.py:650/656.

   • main's tests/unit/test_cli.py, tests/unit/test_suppression.py and tests/nodes/analyzers/test_sc2_command_boundaries.py pass on main (582 passed), where all of these writers are still Path.write_text. Reverting any of them would therefore keep CI green, although each is exploitable: with main's CLI, baseline . in a checkout with a planted .skillspector-baseline.yaml symlink, --mcp-registry --output <symlink> and --recursive --format json --output <symlink> each exit 0 and overwrite the sentinel. The PR body says the baseline CLI race was checked, but no test in the diff exercises a baseline race.

   • Expected fix: one POSIX-only CLI test parametrised over:

     • baseline <skill> --no-llm after chdir into a directory whose .skillspector-baseline.yaml is a relative symlink to a sentinel, with graph.invoke stubbed as at test_cli.py:6988;
     • baseline -o out.json where out.json is a symlink;
     • scan <root> --recursive --format json|sarif|markdown --output <symlink>, with detect_skills and _scan_skill stubbed as at test_cli.py:3672;
     • scan registry.json --mcp-registry --format json --output <symlink>.

     Each case should assert exit 2, a "non-regular" message and an unchanged sentinel. The supported success path is already covered by existing tests (test_cli.py:443, 6988, test_cli_mcp_registry_routes_and_writes_json).

6. [Non-blocking] README.md:760: the user-visible changes are not documented.
   • The PR changes no docs. README.md:760 ("-o, --output PATH Output file path"), the --output help at cli.py:472 and cli.py:3378, the Docker example at README.md:116-121, README.md:239/769 and docs/SUPPRESSION.md:24/37 do not mention the refused targets (Windows, symlinked parents, special files, non-writable parents) or the 0600 mode.
   • Your earlier merged security PRs (#736, #737, #744, #747) updated README or docs for their user-visible changes. A CHANGELOG line is optional; only 2 of 86 first-parent commits since 2.12.0 touched it.
   • Expected fix: short notes in the README CLI-options and Docker sections and in docs/SUPPRESSION.md, reflecting whatever is decided for findings 1-4.
7. [Non-blocking] contrib/batch_scan/batch_scan.py:525: the batch scanner's documented -o still writes with Path.write_text.
   • batch_scan.py:524-525 is unchanged, and the usage is documented at README.md:178. With main's module (identical on the head), -o report.json run inside a directory where report.json is a symlink overwrote the symlink's target with the batch report, and -o hard.json wrote through a hard link.
   • This is not a regression: the PR body scopes the change to CLI writers, contrib ships outside the wheel, and the attacker must control the output directory. It is the same bug class, though, and the PR's one-line summary is unqualified.
   • Expected fix: route that write through write_text_no_follow, catching ValueError/OSError and exiting non-zero as the CLI does, or state in the PR description that contrib writers are out of scope. A follow-up PR is fine.
8. [Non-blocking] tests/unit/test_cli.py:7151: the assertion can fail when Rich wraps the error line.
   • The printed line "Error: Safe file output is unsupported on this platform; use stdout redirection." is exactly 80 characters and goes through err_console = Console(stderr=True) (cli.py:118). With COLUMNS=79, a transcription of the print and the assertion wraps to "use stdout \nredirection." and fails, while "unsupported on this platform" still matches.
   • main's test_cli.py passes at COLUMNS=78 and 79 and already fails at 76 and below, so this only adds failures at 77-79. Default CI (no COLUMNS, no tty) uses width 80.
   • Expected fix: assert on "unsupported on this platform", or normalise whitespace before matching.

PIC tradeoffs:

• Windows output (finding 1). The options are: fail closed before the scan and give baseline a stdout mode; keep main's plain write with a warning on Windows (usable, but the race stays open there); or build a handle-pinned Windows writer (parity, but non-trivial and not exercised by the Linux-only CI). input_handler already has a Windows read path that a writer could mirror.
• Rename-based replace. It protects hard-linked targets and makes output atomic, but it does not preserve mode, owner, ACLs or xattrs, it needs a writable parent, and it presumably cannot replace a single-file bind mount (EBUSY on Linux; known kernel behaviour, not measured).
• Symlinked parent directories. Refusing them matches the input-path policy (validate_local_input_path) and keeps the parent-swap guarantee simple, at the cost of users whose output paths cross symlinked directories. That is acceptable with a clear error and documentation (finding 2).
• 0600 vs umask. Private output matches the Pi extension's existing report policy, but it breaks readability in the documented rootful-Docker flow and for other uids on the same host (finding 3).
• Special files. Refusing /dev/null is simplest; allowing character devices keeps an existing idiom (finding 4).
• When to check. Write-time checks keep the race window minimal. A cheap pre-scan check (platform support, parent walk, leaf type) would avoid discarding a long or LLM-backed scan, at the cost of one extra directory walk; the write-time walk must remain the real guard.

Verification and gaps:

• Baseline on main (main's own code): leaf symlinks through _write_result and dump_baseline overwrote the sentinel; a hard-linked destination was modified in place; --mcp-registry and --recursive --format json through a symlinked --output exited 0 and overwrote the sentinel; the planted .skillspector-baseline.yaml checkout exited 0 and overwrote victim_rc. The batch scanner's -o overwrote both a symlink target and a hard-linked file.
• PR behaviour (my own transcriptions of file_output.py:27-85 and the changed call sites, run with main's venv on macOS / Python 3.13; nothing was imported from the PR):
  • A leaf symlink, a FIFO and a directory at the destination are rejected.
  • A leaf swapped to a symlink just before the replace has only the link entry replaced, and the sentinel is untouched.
  • A parent swapped after open sends the write into the originally opened directory.
  • A hard-linked destination is replaced without modifying the linked file.
  • A failed replace leaves no .skillspector-output-* file.
  • Output bytes match Path.write_text for \n, \r\n and non-ASCII content. Relative paths and the /tmp//var root aliases still work.
  • The planted-baseline checkout now exits 2 with victim_rc intact.
• Gate and errors: the gate is True on darwin / Python 3.13.14 (O_PATH absent, so O_RDONLY is used). Checking os.rename in os.supports_dir_fd is the right proxy for os.replace with dir_fd. The new ValueError/OSError cases reach the existing handlers and exit 2 without a traceback (cli.py:879-888, 707-709, 783-791, 3439-3447). There is no import cycle.
• Tests: the three new tests fail on main, where skillspector.file_output does not exist, and the two helper tests would also fail against a naive Path.write_text helper. The pre-existing tests pass with either implementation of the registry, recursive and baseline writers (finding 5).
• CI: no checks have run. The workflow run on a8c13b6 is action_required (waiting for a maintainer), so the PR description's test counts are unverified, and CI must pass before merge.
• Conflicts: mergeable with main. The PR shares cli.py, suppression.py and tests/unit/test_cli.py with open PR #792 and merges cleanly with it. #792 changes cache-key derivation, _bounded_cache_update and suppression.py 172-206, while this PR touches only the write sites and suppression.py 646-656, so I found no semantic interplay. No trial merge was run.
• Head update: I reviewed a8c13b62eff70de8bd40b180ceba72cf53a790c9. The current head 8b500a1e99738b6bbc539f5c7c024ef5b95ee73b only adds automated merges of main (#691, #577, #608) from update-pr-branches.yml; those commits touch none of this PR's files. The PR's own diff is unchanged (same patch-id), so this review applies to the current head.
• I did not run the PR's tests or code, per policy.
• Gaps: Linux was not run locally (the O_PATH walk is inferred from the identical, CI-tested walk in input_handler). Windows was not run (the gate value comes from the CPython docs, and the failure was simulated by forcing the gate False). Docker was not built or run, so finding 3's root-owned file is inferred. Local Python is 3.13; CI uses 3.12.

Decision: Changes Requested (reviewed head a8c13b62eff70de8bd40b180ceba72cf53a790c9; current head 8b500a1e99738b6bbc539f5c7c024ef5b95ee73b only adds merges of main)

[SkillSpector Review]

Hi @yashrajp22, thank you for the thorough round of fixes. Moving the platform check ahead of the --mcp-registry, --recursive and single-skill branches means an unsupported platform now fails before any analysis. The new baseline -o - mode keeps stdout to pure JSON, so Windows users have a working baseline path. Thanks also for routing contrib/batch_scan through the same anchored writer, with a hard-link case that main's batch writer would fail.

Value and readiness:

• The fix is still needed on main. With main's own code, single-skill scan <skill> --no-llm -f json -o <symlink> exits 0 and overwrites the symlink's target. With my transcription of this PR's writer, the same leaf symlink is refused, naming the path and type 'l', and the target is intact.
• Earlier findings: six of the eight are resolved (1, 2, 3, 6, 7, 8) and two are partly resolved (4, 5). Your thread replies match the code, with one exception: the finding-4 reply says write errors identify the path, which holds for reports but not for baselines.
• Two blockers remain. Neither is a defect in the anchored writer itself.
  • Finding 1: a one-hunk import conflict with #792. A maintainer may push the prepared merge fb2dc65, which resolves only that hunk.
  • Finding 2: resolving the #657 merge removed #657's in-place update for shared baselines. Your security reasoning holds: I reproduced main writing a baseline into a hard-linked unrelated file. But this reverses behaviour the maintainer explicitly asked #657 to keep, so the PIC needs to decide.
• Findings 3-6 are non-blocking.

Previous findings (review posted on 8b500a1):

1. Windows: baseline unusable, and --output failing only after the scan. Resolved. Your reply is accurate.
   • scan checks platform support right after argument validation, before the --mcp-registry, recursive and single-skill branches (src/skillspector/cli.py:671-676).
   • baseline checks inside its try, before graph.invoke, so the ValueError exits 2 (cli.py:3462-3464).
   • The batch scanner checks before discovery (contrib/batch_scan/batch_scan.py:376-381).
   • baseline -o - writes serialize_baseline JSON to stdout (cli.py:3502-3503). Verbose and incomplete-analysis messages go to err_console, and logging goes to stderr (logging_config.py:44). Stdout is reconfigured to UTF-8 (cli.py:90-107).
   • The error message gives command-specific options (file_output.py:38-45), and the limitation is documented (README.md:784-788, docs/SUPPRESSION.md:58-60).
   • Tests: test_unsupported_output_stops_before_analysis covers all five modes (tests/unit/test_cli.py:7270), and test_baseline_stdout_works_without_secure_file_output covers stdout baselines (7298).
   • The stdout substitute has gaps of its own; see finding 5.
2. Symlinked output directory giving a raw ENOTDIR that named one component. Resolved. Your reply is accurate.
   • Walk errors and write errors now name the full requested path (file_output.py:62-71, 122-129). Transcription results:
     • <abs>/plink/r.json, with plink a symlink to a directory: "Could not open output directory for '/plink/r.json': an output directory is a symlink or is not a directory."
     • A missing parent: "... the output directory does not exist."
   • The refusal is documented (README.md:782-783, docs/SUPPRESSION.md:63), and the PR body no longer claims that arbitrary outside paths work.
   • Optional: this error still arrives after the scan, and it does not say which component is the symlink. The PR body's "Errors identify the requested path and invalid parent" is therefore slightly overstated.
3. Always 0600, including rewrites; rootful Docker. Resolved (policy kept and documented). Your reply is accurate.
   • The 0600 policy is documented (README.md:779-781, docs/SUPPRESSION.md:61-62).
   • The Docker example passes --user "$(id -u):$(id -g)" (README.md:119).
   • Existing baselines keep their owner, group, mode and ACLs (suppression.py:828-848).
   • Transcription under umask 022: a new report is 0600, an existing 0644 report becomes 0600 (documented), a new baseline is 0600, and an existing 0644 baseline stays 0644.
4. Refusals without a path, and newly refused targets. Partly resolved. Your reply is accurate for reports only.
   • Fixed for reports:
     • Refusals name the path and file type (file_output.py:94-98), for example "Refusing to overwrite non-regular output '/dev/null' (type 'c')."
     • Write errors are re-raised with the path (122-129), for example "Could not write output '/rodir/w.json': Permission denied."
     • Refused devices, the writable-parent requirement and readable macOS ancestors are documented (README.md:781-784, docs/SUPPRESSION.md:61-64).
   • Still open: baseline write errors still name a bare file or temp name. See finding 3.
5. No tests for the baseline, recursive and registry writers. Partly resolved. Your reply is accurate about what was added.
   • Added:
     • test_all_cli_writers_reject_symlink_destinations (test_cli.py:7333) covers default and JSON baselines, --mcp-registry, and recursive json, sarif and markdown. Each case asserts exit 2, the refusal text and an intact sentinel.
     • Baseline parent-swap and hard-link races are covered by test_report_output_cannot_redirect_to_external_file[baseline-*] (7161).
     • Batch is covered at tests/test_batch_scan_security.py:547.
   • The two baseline cases at 7333 would also pass on main, because #657's writer already refuses leaf symlinks. The 7161 races are what cover the new anchoring.
   • Still open: the single-skill writer lost its only test. See finding 4.
6. User-visible changes undocumented. Resolved (no inline thread).
   • Added README.md:116-121 and 779-788, and docs/SUPPRESSION.md:58-64 and 136-147.
   • The stdout guidance needs the caveats in finding 5.
7. contrib/batch_scan still using Path.write_text. Resolved. Your reply is accurate.
   • batch_scan.py:376-381 checks support before scanning. Lines 535-540 write through write_text_no_follow and exit 2 on OSError or ValueError.
   • The test at tests/test_batch_scan_security.py:547 covers the symlink, hard-link and unsupported cases. On main's module, -o through a hard link writes into the linked file, so the hard-link case discriminates.
   • The other hunks in the file are formatting only.
   • Pre-existing and out of scope: under Rich, _print drops file= (batch_scan.py:298-301), so the new Error: lines go to stdout. Open PR #755 fixes this.
8. Rich wrapping of the 80-character error line. Resolved. Your reply is accurate.
   • Both assertions normalise whitespace (test_cli.py:7260, 7293).
   • The early-failure test sets COLUMNS=79 (7275).

Material findings

1. [Blocker] src/skillspector/cli.py:86: the PR conflicts with main after #792 (one import hunk).
   • Evidence:
     • GitHub reports 102bec18 as CONFLICTING (DIRTY) against main 5edc347c.
     • git merge-tree --write-tree origin/main <head> gives exactly one conflict hunk, in the from skillspector.suppression import (...) list: this PR adds serialize_baseline, (cli.py:86), and #792 adds source_content_key, (main cli.py:85).
     • suppression.py, tests/unit/test_cli.py and tests/unit/test_suppression.py auto-merge. #792's other changes (source-scoped cache keys and _bounded_cache_update) do not touch this PR's lines.
   • The prepared merge fb2dc65:
     • Its parents are exactly 102bec18 and 5edc347c.
     • It differs from the merge-tree result only in that hunk: the markers are dropped and the list reads serialize_baseline, then source_content_key,.
     • Its diff against main has the same 9 files, +650/-212, as the PR's own diff.
     • Run with main's config, ruff check passes on its changed files and ruff format --check passes on cli.py (a static check only).
     • The maintainer reports lint clean and 537 tests passing. I did not re-run the tests.
   • Consequence: the PR cannot merge as is.
   • Expected fix: merge main, keep both names in that order, and change nothing else. Alternatively, a maintainer may push fb2dc65, for example with git push origin fb2dc65:refs/heads/yashraj/fix-cli-report-output-race; the review bot will not push it. Merging does not settle finding 2, because fb2dc65 keeps this PR's refusal.
2. [Blocker] src/skillspector/suppression.py:802: resolving the #657 merge removed #657's shared-baseline update path. This needs an explicit PIC decision.

   • Evidence:

     • On main, _write_baseline_in_place (main suppression.py:729-750) rewrites the validated inode in place in two cases: a non-owner writer (824-832), and an owner who cannot assign the file's group (850-861). This is documented at main docs/SUPPRESSION.md:133-138.
     • This PR raises PermissionError in both cases instead (suppression.py:802-808 and 833-839), and says so in docs/SUPPRESSION.md:142-145 and README.md:779-781.
     • In tests/unit/test_suppression.py, it inverts and renames two #657 tests: 1180 and 1208, which on main are shared_writer_preserves_inode_and_group_access and owner_outside_destination_group_rewrites_in_place. It deletes test_dump_baseline_shared_writer_revalidates_destination_replaced_before_lock without a replacement.

   • History:

     • Regenerating a colleague-owned baseline has always worked on main. Before #657, dump_baseline used p.write_text (91080092^ suppression.py:649, 655).
     • #657 deliberately kept that case working (alice:devs 0664 regenerated by bob) at the maintainer's request (#657 (comment)).
     • #657 merged at 2026-10-09 06:17 UTC, and this branch absorbed it in afbb16b2 at 08:17 UTC. This PR is the first change to remove the behaviour.
     • The PR body and docs disclose the refusal, so the change is not hidden. The PR body does not say that it reverses #657.

   • Measured. The cases were simulated on one uid by patching os.geteuid, or fstat/fchown for the group case. The main side ran main's code; the PR side is my transcription of suppression.py:772-839.

     Case	main	This PR (transcribed)
     Shared 0664 baseline, non-owner writer	Rewritten in place (same inode, new content)	Refused; file unchanged
     Shared 0664 baseline, owner who cannot assign the group	Rewritten in place	Refused; file unchanged
     Destination hard-linked to an unrelated 0664 file (both cases above)	Baseline written into the unrelated file	Refused; both files unchanged

     So your rationale holds.

   • Consequence: a team member who regenerates a colleague-owned baseline now gets exit 2 where main succeeds. Examples are a setgid group directory or mixed-uid CI. The tests that encoded the maintainer's request are also gone.

     • The hard-link exposure on main is narrow. An attacker needs write access to the baseline directory on the same filesystem, plus a file that the regenerating user can write but does not own.
     • Linux fs.protected_hardlinks=1 narrows it further (kernel behaviour; not measured).

   • Expected fix: a PIC decision between two options.

     • (a) Accept the refusal. In that case, please state in the PR body that it reverses #657's shared-writer fallback.

     • (b) Keep shared regeneration. Restore #657's in-place path and its tests within the anchored design:

       • revalidate with os.stat(absolute.name, dir_fd=directory_fd, follow_symlinks=False) instead of main's p.lstat();
       • refuse when os.fstat(descriptor).st_nlink != 1;
       • add a hard-link refusal test.

       A sketch of main's code plus that guard kept both shared cases working and refused both hard-link cases. Two limits remain. Readers can still see a partial write, as main already documents. A victim file renamed into place still has st_nlink == 1, although renaming it there needs write access to the victim's own directory.

3. [Non-blocking] src/skillspector/suppression.py:792: baseline write errors name a bare file or temp name instead of the output path.
   • Evidence:
     • Inside with open_output_parent(p), dump_baseline opens the destination (792) and the temp file (819-824) by bare name with dir_fd. It does not wrap the resulting OSErrors; write_text_no_follow does (file_output.py:122-129).
     • The baseline command prints these through its generic handler (cli.py:3514-3519).
     • Transcription compared with main's dump_baseline:
       • A 0444 <dir>/b444.yaml gives "[Errno 13] Permission denied: 'b444.yaml'". main names '<dir>/b444.yaml', as the user spelled it.
       • A writable baseline in a 0555 directory gives "Permission denied: '.skillspector-baseline-.tmp'". main names '<dir>/.skillspector-baseline.<rand>.tmp'.
     • The byte-limit error also lost its path (suppression.py:756); main ends that message with : {p} (main suppression.py:792).
     • The three hand-built errors (788, 803-808, 834-839) do include str(p).
   • Consequence: when the output path has a directory component, the error no longer shows the directory. That is a regression from main, it differs from report errors in the same PR, and it contradicts the PR body's "Errors identify the requested path".
   • Expected fix: wrap the body of with open_output_parent(p) the way write_text_no_follow does. Re-raise OSError with the path, for example ValueError(f"Could not write baseline {str(p)!r}: {exc.strerror or exc}"). The existing PermissionErrors can be left out of the wrapping or wrapped harmlessly. Optionally, carry the path into the byte-limit message for file output.
4. [Non-blocking] tests/unit/test_cli.py:7333: the single-skill report writer lost its only regression test.

   • Evidence:

     • _write_result (cli.py:356, write at 364) serves scan <skill> -o report.json, the most common output path.
     • In round 1, test_report_output_unsupported_platform_keeps_stdout reached it. With the early gate (cli.py:671-676), that test (7247) and test_unsupported_output_stops_before_analysis[single] (7270) both exit before _write_result.
     • The 7333 parametrisation has no single-skill mode; its json, sarif and markdown modes are recursive.
     • The symlink and hard-link cases at 474-487 stop at the pre-scan input-alias guard. test_cli_scan_preserves_input_when_writing_a_separate_report uses only regular files.
     • No test references _write_result.

   • Consequence: reverting cli.py:364 to Path(output).write_text would keep CI green and reopen the overwrite. I inferred that from reading the tests and did not run a mutation. On main, which has that form, scan <skill> --no-llm -f json -o <symlink> exited 0 and overwrote the symlink's target (measured with main's CLI).

   • Expected fix: add a single mode to the 7333 test:

     • run scan <tmp_path>/skill --no-llm -f json --output <symlink>;
     • stub _scan_skill with monkeypatch.setattr(cli, "_scan_skill", lambda **kw: {"report_body": "report", "risk_score": 0});
     • assert exit 2, "non-regular" in the output, the sentinel still "preserve", and output.is_symlink().

     Pointing at the skill directory avoids the input-alias guard. Optionally, add a hard-link case asserting that the linked file is unchanged.

5. [Non-blocking] docs/SUPPRESSION.md:58: the documented stdout substitute for baseline files has gaps.

   • Evidence:

     • dump_baseline always enters open_output_parent (suppression.py:772). On Windows, the only baseline flow is therefore -o - plus shell redirection (docs/SUPPRESSION.md:58-60, README.md:786-788). main's writer has a non-POSIX branch (main suppression.py:871-872).
     • These gaps were measured with main's CLI and loader unless noted:
       • (a) Truncation. The shell truncates the target before the command runs. sh -c 'skillspector baseline <missing> --no-llm -o - > existing.json' exited 2 and left existing.json at 0 bytes. An empty file then loads, with a warning, as a legacy baseline with 0 rules, so the next scan reports every finding again.
       • (b) Encoding. Windows PowerShell 5.1's > writes UTF-16LE with a BOM (per PowerShell's documentation; not run). PowerShell 7 defaults to UTF-8. load_baseline rejects a UTF-16LE file ("can't decode byte 0xff in position 0"), so scan --baseline exits 2. A UTF-8 file with a BOM loads.
       • (c) POSIX shared files. The refusal messages (suppression.py:805-806, 836-837), docs/SUPPRESSION.md:143-145 and the PR body send non-owners of a shared baseline to --output -. Redirecting that output to the same shared file (-o - > shared.yaml) truncates and rewrites the shared inode in place, non-atomically and through any hard link. That is exactly what the code refuses (I traced this by reading the code; not separately run).
       • (d) Batch stdout. README.md:786 says to omit --output for batch stdout. But batch -f json stdout starts with a "SkillSpector Batch Scan" header and is not valid JSON (measured on main's module, whose stdout handling this PR leaves unchanged). Open #755 fixes this.
     • Minor: with -o -, no baseline_path is set (cli.py:3470-3471, asserted at test_cli.py:7325). An old baseline left inside the scanned tree is therefore scanned; in my run it produced 2 fingerprints against itself. This only affects the "write to a new file" flow, and the fingerprints are inert once the old file is replaced.

   • Consequence: users who follow the docs can wipe a committed baseline on any failure, or get a file that the scanner refuses in the default Windows shell. Both (a) and (b) fail loudly rather than hide findings.

   • Expected fix: document a safe flow:

     • redirect to a new file outside the scanned tree;
     • replace the old baseline only after exit 0;
     • in Windows PowerShell 5.1, use cmd /c or | Out-File -Encoding utf8;
     • note the batch stdout limitation until #755 lands.

     Also change "or use --output -" in the refusal messages and docs to "or use --output - redirected to a new file". Optionally, have load_baseline accept BOM-marked UTF-16. Alternatively (a PIC call), keep #657's path-based writer as the fallback when _SECURE_OUTPUT_SUPPORTED is False.

6. [Non-blocking] src/skillspector/file_output.py:94: an existing read-only report is silently replaced, while baselines refuse the same case.

   • Evidence:

     • write_text_no_follow checks only S_ISREG on an existing destination (89-98). It then renames a new 0600 file over it (99-114), which needs a writable directory but not a writable file.
     • Test case: a 0444 r444.json and a non-root user.
       • main's CLI (scan <skill> --no-llm -f json -o r444.json) exited 2 with Permission denied and left the file unchanged.
       • My transcription of this writer replaced the content and left the file at 0600.
     • This PR's dump_baseline refuses a 0444 baseline because it opens the existing file O_WRONLY first (suppression.py:791-792). That refusal is documented (docs/SUPPRESSION.md:137-138) and tested (tests/unit/test_suppression.py:1335).
     • README.md:779-784 says nothing about read-only reports.

   • Consequence: a report protected only by its mode bits is overwritten without warning, and reports and baselines handle the case differently.

     • A non-writable parent still protects the report, and so does macOS uchg: in the transcription, both were refused with the path.
     • Root behaves as on main.

   • Expected fix: when the destination exists, probe it before writing:

     • open it with os.open(absolute.name, os.O_WRONLY | os.O_NONBLOCK | os.O_NOFOLLOW, dir_fd=directory_fd);
     • re-check S_ISREG with os.fstat, then close it;
     • refuse with the output path on PermissionError.

     Alternatively, document that report output replaces read-only files.

PIC tradeoffs:

• Shared baselines (finding 2). The refusal keeps every baseline write atomic and off hard-linked inodes. The cost is the shared-group regeneration that the maintainer asked #657 to keep. Option (b) restores it with an st_nlink guard, but readers can still see a partial write.
• Windows baselines (finding 5). The choice is between failing closed with a stdout substitute and keeping #657's path-based writer on platforms without dir_fd support. That writer is atomic and checks for leaf symlinks, but it does not pin parent directories.
• Read-only reports (finding 6). The options are to replace them (current behaviour) or to refuse them, as baselines and main do.
• Already documented and acceptable:
  • reports are always 0600, while existing baselines keep their mode;
  • symlinked parents and devices (-o /dev/null) are refused where main allowed them.

Verification and gaps:

• Head. I re-checked the live head with gh before writing: it is 102bec18a6189c41f0736a2a4cab92a41d80a976. Its PR diff has the same patch-id as 57a63d50; 102bec18 only adds an automated merge of main up to #791.
• CI.
  • All six checks passed on 57a63d50: lint, test-unit, docker-smoke, DCO Check, changes and OpenCode TypeScript Tests.
  • On 102bec18, the only run is action_required, and no checks ran.
  • CI must pass on the post-merge head before merge.
• Conflicts with main: see finding 1.
• Other open PRs, each trial-merged against fb2dc65:
  • #755 conflicts in contrib/batch_scan/batch_scan.py, right after args = parser.parse_args(). Keep both blocks, and put this PR's support check after #755's _print redefinition so the error goes to stderr.
  • #796 and #797 each have an append/append conflict at the end of tests/test_batch_scan_security.py. Keep both test blocks.
  • #758 adds a pre-scan samefile guard for --mcp-registry inputs. It merges cleanly and complements this PR's write-time guard.
  • #795, #806 and #617 merge cleanly.
  • #741 and #793 touch none of this PR's files.
• Method. PR-side behaviour comes from my own hand transcription of file_output.py:30-129 and the I/O parts of suppression.py:762-864, run with main's venv on macOS (Python 3.13.14). main-side results come from main's own code. I did not run the PR's tests or code, per policy.
• Gaps:
  • Linux was not run locally. The O_PATH walk relies on CI passing on 57a63d50 with the identical diff.
  • Windows was not run. The PowerShell encoding behaviour comes from its documentation, and unsupported-platform behaviour was assessed by reading the code.
  • The non-owner and group cases were simulated by patching os.geteuid, fstat and fchown, because no second uid was available.
  • The maintainer's 537-test result for fb2dc65 and the PR body's 529 and 6,243 test counts were not re-run.
  • Docker with --user was not run.

Decision: Changes Requested (reviewed head 102bec18a6189c41f0736a2a4cab92a41d80a976)

[SkillSpector Review]

@yashrajp22 thank you for the anchored-writer hardening, and for your patience while this PR waits on a maintainer decision.

rng1995 pushed the merge commit fb2dc658 to this branch, so the review bot will not approve this PR. A human maintainer must approve it once the blockers are cleared.

Value and readiness

This is a status re-review. The PR's own diff has not changed since the 2026-10-12 review. Its only new commit is the maintainer-pushed #792 merge fb2dc658, and CI passed on it. Since then, #796 and #755 have landed on main, so the PR conflicts with main again. The PIC decision on shared baselines is also still open. The PR is not ready.

Material findings

Prior findings:

1. Blocker, resolved. src/skillspector/cli.py:86: fb2dc658 resolves the #792 import conflict, and cli.py now auto-merges with main.
2. Blocker, still open. src/skillspector/suppression.py:802 (open thread): this PR reverses #657's in-place update of shared baselines. The PIC must decide whether to keep that change, and no decision is recorded.
3. Non-blocking, still open:
   • suppression.py:792: baseline write errors name only the bare file name.
   • test_cli.py:7333: no regression test covers the single-skill writer.
   • file_output.py:94: read-only reports are silently replaced.
4. Non-blocking, partly resolved by main. docs/SUPPRESSION.md:58:
   • Resolved: #755 made batch stdout report-only, which settles gap (d).
   • Still open: gaps (a) to (c).

New blocker: conflicts with main 8b993d32.

• contrib/batch_scan/batch_scan.py (#755): _print is now local to _main_impl and is defined after parse_args(). Keep both blocks, and put this PR's require_secure_file_output() check after that definition. Calling _print earlier raises UnboundLocalError.
• tests/test_batch_scan_security.py: #796's tests and this PR's tests both append to the end of the file. Keep both blocks.

A maintainer may push a prepared merge again. The bot will not push one.

PIC tradeoffs

Unchanged:

• Shared baselines: refuse the in-place update, or restore it behind an st_nlink guard.
• Windows baselines: stdout is the only flow there.
• Read-only reports: replace them, or refuse them.

Verification and gaps

• I re-ran git merge-tree against current main and read both conflict regions. The other files auto-merge.
• main's cli.py changes since 5edc347c add no new file writer.
• The author has not replied or pushed since the last review, and all five threads are still unresolved.
• CI passed on fb2dc658 and must run again after the merge.
• This was a static review.

Decision: Changes Requested (reviewed head fb2dc658bb4598ae7b37b16b183cdcb84e7689ed)